PB-1083: Removed default icon set 'babs' from UNLISTED_ICON_SET #93
Conversation
This change helps us avoid changing UNLISTED_ICON_SET's content on both the service and the k8s configuration. In order to achieve this, I'm assigning an empty string '' (since os.environ.get requires a default value), and removing that string with a simple loop.
app/settings.py
Outdated
| UNLISTED_ICON_SETS = os.environ.get('UNLISTED_ICON_SETS', '').split(',') | ||
| UNLISTED_ICON_SETS = [icon_set for icon_set in UNLISTED_ICON_SETS if icon_set] |
There was a problem hiding this comment.
can we bunch these two lines into one?
also if icon_set might not be enough to filter out a blank value given by the os.environ.get as the default there is ''. Maybe something like if not icon_set == '' ?
There was a problem hiding this comment.
We could bunch these lines and have something like :
UNLISTED_ICON_SETS = [icon_set for icon_set in os.environ.get('UNLISTED_ICON_SETS', '').split(',') if icon_set]
But I find that very hard to read, and even my original implementation is far from perfect. I would like to write a function fetchAndCleanUnlistedSets that returns a clean UNLISTED_ICON_SETS, therefore making settings.py more readable. I'm just not sure which file I should write it (if I should write the function that is)
From my testing, if icon_set correctly filters out the blank and UNLISTED_ICON_SETS comes out as an empty array ("UNLISTED_ICON_SETS = []"). Empty strings are considered "false" in python so I don't see why it wouldn't work.
if not icon_set == '' would also work, but is more verbose and less readable in my mind (I usually favor positive conditions as we are quicker to grasp them). For what reason should we favor it over the other?
There was a problem hiding this comment.
a possibility to make a one line would be the following :
UNLISTED_ICON_SETS = [icon_set for icon_set in os.environ.get('UNLISTED_ICON_SETS', '').split(',') if os.environ.get('UNLISTED_ICON_SETS')]
the default for os.environ.get('') is none, which means we would have an empty array if the environment variable is not set up. With the current implementation, you'd get an array containing ''
There was a problem hiding this comment.
a possibility to make a one line would be the following :
UNLISTED_ICON_SETS = [icon_set for icon_set in os.environ.get('UNLISTED_ICON_SETS', '').split(',') if os.environ.get('UNLISTED_ICON_SETS')]the default for
os.environ.get('')isnone, which means we would have an empty array if the environment variable is not set up. With the current implementation, you'd get an array containing''
At this point it's better to have a separate function I would say, that's too much for one line imo
This change improves readability of settings.py, and removes the double assignement, with a cleaner overall routine. UNLISTED_ICON_SETS is now assigned in settings.py using the function fetch_and_clean_unlisted_sets.
|
|
||
| def fetch_and_clean_unlisted_sets(): | ||
| unlisted_icon_sets = os.environ.get('UNLISTED_ICON_SETS', '').split(',') | ||
| cleaned_unlisted_icon_sets = [icon_set for icon_set in unlisted_icon_sets if icon_set] |
There was a problem hiding this comment.
Have you tested how this behaves when it receives nothing through the UNLISTED_ICON_SETS env variable?
I'm still not quite comfortable with this last if icon_set and the default value being an empty string...
This change helps us avoid changing UNLISTED_ICON_SET's content on both the service and the k8s configuration.
In order to achieve this, I'm assigning an empty string '' (since os.environ.get requires a default value), and removing that string with a simple loop.
[X] Local tests pass
[X] Manual testing done on both docker and local server